fix: four BUG-severity findings from the 2026-07-02 deep review (I143–I146) - #89
Merged
Conversation
…–I146) I143 — TOCTOU create-vs-open race (src/lib.rs): the create_new vs open_existing decision was made from an unlocked path.exists() stat taken BEFORE the flock. A process racing open() could stat an empty file while another creates + commits + releases the lock, then run create_new over the committed data. Decide from io.page_count() (cached from the post-lock file length) instead; the pre-lock stat is kept only for the create_if_missing gate, which must stay pre-lock so a refused open never materializes a file. I144 — unvalidated crypto-header stride (src/transaction/recovery.rs): the plaintext stride (guarded only by the forgeable XXH3 checksum) went verbatim into set_stride, which computes file_len / stride. A forged stride=0 was a division-by-zero panic (violating poison-not-panic); a huge value drove multi-GiB allocations. Require stride == ENC_PAGE_SIZE before set_stride; mismatch returns CorruptSuperblock. Regression test forges the on-disk stride to 0, re-stamps the checksum, and asserts the reopen errors, not panics. I145 — reclaim_freemap_orphans not poison-wrapped (src/transaction/freemap.rs): a fatal error mid-sweep returned un-poisoned, and reclaim_orphans writes the partially-advanced freemap root back into current_roots even on its error path. Wrap the sweep in poison_on_fatal like every other TM entry point. I146 — README contradicted the code (README.md): the format-compatibility section said the newer-minor write-refusal gate was "not yet wired up"; it shipped as I29 and THEORY.md documents it as live. Corrected to match. Full test suite (incl. the new forge test), clippy --workspace -D warnings, and fmt --check all clean.
Xof
added a commit
that referenced
this pull request
Jul 2, 2026
…ES.md (#90) Adds the fresh-eyes review report (docs/reviews/review-20260702-001902.md) and triages its verified findings as I143–I160 under a new "Deep review 2026-07-02" section in ISSUES.md. - 18 findings confirmed by adversarial verification (0 refuted): 4 BUG, 13 DESIGN, 1 SMELL. Prior-review delta: 0 regressions, 71 resolved. - I143–I146 (the BUGs) are marked FIXED (PR #89). - I147–I160 (DESIGN/SMELL) recorded OPEN for triage — corrupt-page hardening asymmetry, KDF-param DoS, per-page replay, the encrypted-minor write-gate off-by-series, Python finished-transaction aliasing, and the encrypted on-disk-spillway test gap are the notable ones. Docs only; no code change.
🚦 Bench results: PR vs main
Per-scenario detail (4 metrics × cells)document-store
mutation-log
ycsb-a
ycsb-b
|
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes the four BUG-severity findings from the fresh-eyes deep review (2026-07-02), each verified against the code by an independent adversarial pass. Tracked as I143–I146 in ISSUES.md (added in the companion review PR).
Fixes
I143 — TOCTOU create-vs-open race (
src/lib.rs)Chisel::openchosecreate_newvsopen_existingfrom an unlockedpath.exists()stat taken before theflock. Two processes racingopen()could have B stat an empty/absent file, A concurrently create + commit + release the lock, then B runcreate_newover A's committed data. Now the create/open decision usesio.page_count()— the count cached from the file length observed after the lock is held. The pre-lock stat is retained only for thecreate_if_missinggate (which must stay pre-lock so a refused open never materializes an empty file).I144 — unvalidated
stride→ panic / DoS on open (src/transaction/recovery.rs)The encrypted crypto-header's
stride(plaintext, guarded only by the forgeable XXH3 checksum) was passed verbatim toset_stride, which computesfile_len / stride. A forgedstride = 0was a guaranteed division-by-zero panic (violating the poison-not-panic model); a huge value drove multi-GiB read allocations. Nowstridemust equalENC_PAGE_SIZEbeforeset_stride; a mismatch returnsCorruptSuperblock. This also makes thecrypto_header.rs"validated by the engine" comment true. Regression test:corrupt_crypto_header_stride_errors_not_panicforges the on-disk stride to 0, re-stamps the checksum, and asserts the reopen returnsCorruptSuperblockrather than panicking.I145 —
reclaim_freemap_orphansnot poison-wrapped (src/transaction/freemap.rs)Every other
TransactionManagerentry point routes fatal errors throughpoison_on_fatal; the orphan sweep did not. A fatal error mid-sweep returned un-poisoned, andreclaim_orphanswrites the partially-advanced freemap root back intocurrent_rootseven on its error path — leaving a usable manager holding an indeterminate freemap. Now wrapped inpoison_on_fatallike the rest (inner cache borrow scoped so it's released beforepoison_on_fataltakes&self).I146 — README contradicted the code on a durability contract (
README.md)The format-compatibility section said the newer-minor write-refusal gate was "not yet wired up." It shipped as I29 (
recovery.rs, forces the handle read-only so mutations returnReadOnlyMode), and THEORY.md already documented it as live. Corrected the README to match the code and its sibling doc.Not in this PR
The 13 DESIGN findings and 45 SMELL/NIT from the same review are recorded in the companion review PR (review file + ISSUES.md triage) for separate triage, not fixed here.
Verification
cargo build, fullcargo test(incl. the new forge test),cargo clippy --workspace -- -D warnings, andcargo fmt --checkall clean. No behavior change on the plaintext path; the I143 change is guarded by every existing reopen test (data preserved ⇒open_existingchosen).